Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Speed up #119

Merged
merged 11 commits into from
Sep 13, 2024
Merged

Speed up #119

merged 11 commits into from
Sep 13, 2024

Conversation

gravesti
Copy link
Collaborator

@gravesti gravesti commented Sep 4, 2024

From our discussion yesterday @audreyyeoCH about performance problems, I reviewed the code.
I found there is significant potential to speed up some of the distribution calculations. I have adapted some old code of mine implementing beta mixtures.
I believe there is still potential for improvement.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 4, 2024 •
edited
Loading

badge

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  ----------------------------------
R/betadiff.R                59       0  100.00%
R/boundsPostprob.R          31      31  0.00%    32-71
R/boundsPredprob.R          28      28  0.00%    36-73
R/dbetabinom.R              81       3  96.30%   32, 62, 136
R/oc2.R                    162     162  0.00%    93-326
R/oc3.R                    146     146  0.00%    91-308
R/ocPostprob.R              99       4  95.96%   239-242
R/ocPostprobDist.R         109       0  100.00%
R/ocPredprob.R             182       4  97.80%   285-288
R/ocPredprobDist.R         235       3  98.72%   346-348
R/ocRctPostprobDist.R      154       4  97.40%   236-239
R/ocRctPredprobDist.R      283      55  80.57%   207-222, 225-240, 373-376, 403-421
R/plotBeta.R               103      90  12.62%   60-167
R/plotBounds.R              52      52  0.00%    33-90
R/plotDecision.R            79      79  0.00%    16-136
R/plotOc.R                  15      15  0.00%    16-36
R/postprob.R                34       1  97.06%   106
R/postprobDist.R            77       1  98.70%   204
R/predprob.R                24       0  100.00%
R/predprobDist.R           140       1  99.29%   269
R/runShinyPhase1b.R          4       4  0.00%    8-13
R/sumbetadiff.R             63      63  0.00%    24-101
R/sumTable.R                20      20  0.00%    25-48
TOTAL                     2180     766  64.86%

Diff against main

Filename            Stmts    Miss  Cover
----------------  -------  ------  -------
R/dbetabinom.R        +24       0  +1.56%
R/postprob.R           +5       0  +0.51%
R/postprobDist.R       -1       0  -0.02%
TOTAL                 +28       0  +0.46%

Results for commit: 1e8b717

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 4, 2024 •
edited
Loading

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
ocPostprob 💚 $52.66$ $-28.96$ $0$ $0$ $0$ $0$
ocPostprobDist 💚 $2.07$ $-1.61$ $0$ $0$ $0$ $0$
ocPredprob 💚 $8.34$ $-4.41$ $0$ $0$ $0$ $0$
ocPredprobDist 💚 $451.25$ $-401.40$ $0$ $0$ $0$ $0$
ocRctPostprobDist 💚 $3.14$ $-2.50$ $0$ $0$ $0$ $0$
ocRctPredprobDist 💚 $172.15$ $-144.95$ $0$ $0$ $0$ $0$
predprobDist 💚 $9.55$ $-8.37$ $0$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
dbetabinom 👶 $+0.01$ dbetaMix_handles_edge_cases
dbetabinom 👶 $+0.01$ pbetaMix_works_for_edge_cases
ocPostprob 💚 $20.57$ $-11.84$ ocPostprob_gives_results_that_are_within_range_to_stats_pbinom
ocPostprob 💚 $11.45$ $-6.74$ the_PrEfficacy_increases_with_increase_Efficacy_looks
ocPostprob 💚 $13.17$ $-7.88$ the_PrFutility_increases_with_increase_futility_looks
ocPostprob 💚 $4.18$ $-2.33$ the_sum_of_Eff_Fut_Gray_zone_probabiliy_is_1
ocPredprob 💚 $7.58$ $-4.06$ the_PrEfficacy_increases_with_increase_Efficacy_looks
ocPredprobDist 💚 $108.18$ $-95.46$ ocPredprobDist_gives_correct_result_and_list_when_relativeDelta_FALSE
ocPredprobDist 💚 $126.84$ $-113.30$ ocPredprobDist_gives_correct_result_and_list_when_relativeDelta_TRUE
ocPredprobDist 💚 $48.60$ $-43.32$ ocPredprobDist_gives_higher_PrEfficacy_with_more_efficacy_looks
ocPredprobDist 💚 $167.60$ $-149.31$ ocPredprobDist_gives_higher_PrFutility_with_more_futility_looks
ocRctPostprobDist 💚 $1.50$ $-1.23$ ocRctPostprobDist_gives_higher_PrFutility_with_decreased_pE
ocRctPredprobDist 💚 $68.69$ $-57.87$ ocRctPredprobDist_gives_higher_PrEfficacy_with_increased_pE
ocRctPredprobDist 💚 $68.72$ $-57.87$ ocRctPredprobDist_gives_higher_PrFutility_with_decreased_pE
ocRctPredprobDist 💚 $34.67$ $-29.20$ the_sum_of_Eff_Fut_Gray_zone_probability_is_1
postprob 👶 $+0.01$ postprob_works_with_vector_x
postprobDist 👶 $+0.01$ postprobDist_works_with_vector_x
predprobDist 💚 $1.46$ $-1.20$ h_predprobdist_gives_correct_list
predprobDist 💚 $2.85$ $-2.54$ h_predprobdist_gives_higher_predictive_probability_when_thetaT_is_lower
predprobDist 💚 $2.92$ $-2.61$ predprobDist_gives_higher_predictive_probability_when_thetaT_is_lower_in_a_single_arm_trial
predprobDist 💚 $1.43$ $-1.27$ predprobDist_gives_the_correct_results_in_a_two_arm_study

Results for commit 4db2bbf

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 4, 2024 •
edited
Loading

Unit Tests Summary

    1 files     13 suites   1m 48s ⏱️
  115 tests   115 ✅ 0 💤 0 ❌
1 232 runs  1 232 ✅ 0 💤 0 ❌

Results for commit 1e8b717.

♻️ This comment has been updated with latest results.

@danielinteractive
Copy link
Collaborator

Great, thanks a lot @gravesti !!
Can you double check that the parts you refactor are already unit test covered? (I think so, just checking)

gravesti
gravesti commented Sep 4, 2024
View reviewed changes
R/dbetabinom.R Outdated Show resolved Hide resolved
@gravesti
Copy link
Collaborator Author

gravesti commented Sep 4, 2024

Great, thanks a lot @gravesti !! Can you double check that the parts you refactor are already unit test covered? (I think so, just checking)

Yes, will do

@gravesti
Copy link
Collaborator Author

I think this is ready to merge now. @danielinteractive @audreyyeoCH

danielinteractive
danielinteractive reviewed Sep 12, 2024
View reviewed changes
Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @gravesti , one more question, I would love to avoid the skipchecks thing if it does not save a lot of time...

R/dbetabinom.R Outdated Show resolved Hide resolved
@gravesti gravesti merged commit ef3fbc6 into main Sep 13, 2024
24 checks passed
@gravesti gravesti deleted the speed_up branch September 13, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielinteractive danielinteractive danielinteractive approved these changes

@audreyyeoCH audreyyeoCH Awaiting requested review from audreyyeoCH

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gravesti @danielinteractive